-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
disable loading page feature #58
base: main
Are you sure you want to change the base?
Conversation
related to #57 |
Any chance this could be a workaround for issue #56 ? |
@accforgithubtest add the options |
Thanks @gi4no - I just tried it with filebrowser and it seems to help. Previously filebrowser was having the issue #56. Any ideas for when this pr might be merged ? Thanks again ! |
@gi4no - Is it possible to have a global setting for I think that will be useful to have one global config to disable loading page for all applications in CN instead of adding the config for each and every config. Just a suggestion if it can be done. Thanks ! |
…atusReadyChecking
i've add the global config, set |
Thanks for this, I will try to find some time to review and merge it this weekend. |
public async isContainerRunning():Promise<boolean> { | ||
let interval:NodeJS.Timer; | ||
return new Promise((resolve) => { | ||
interval = setInterval(async () => { | ||
if (this.containerRunning) { | ||
resolve(true); | ||
clearInterval(interval); | ||
} | ||
}, 250); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not guaranteed to always work in my opinion. It might fail if the containers webserver needs some time to start and isn't ready yet to accept a connection.
This is the reason we have used requests & status code validations before redirecting.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the containerRunning
because it's set to true
in the checkContainerReady
method (see https://github.com/ItsEcholot/ContainerNursery/blob/main/src/ProxyHost.ts#L161), so I think it's appropriate to use it. Additionally, I've been running some tests with my container, and it works fine for me. let me know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this reply.
containerRunning
does indeed guarantee that the container is running, but this does not guarantee that the application inside the container (in our case most likely a webserver of some sorts) has started up and is ready to accept connections.
While this might work fine for a lot of containers it is in my opinion brittle.
You would need to implement an actual request check (as we already do on the waiting page) to ensure the webserver is ready for our request.
This PR adds an option to disable the default loading page. With this option enabled, the request will hang until the container is ready. Then, the response will be the real response of the container, not the loading page.
In my case, I have an API server. When I call the API, the 200 HTTP response used to be the content of the HTML loading page. With this feature, the 200 response will be the actual server response.
added global config
disableDefaultLoadingPage
for disable all loading pageadded proxy config
enableDefaultLoadingPage
for enable loading page if thedisableDefaultLoadingPage=true
added proxy config
customHttpStatusReadyChecking
for have custom http status for ready check of the containerI would like to thank you of this library for their hard work and dedication. This change will be a valuable addition to the library.
Let me know what u think!
Thank you!